Skip to content

feat: add Shutdown method#9

Merged
nnnkkk7 merged 2 commits intomainfrom
add/shutdown-method
Jul 2, 2025
Merged

feat: add Shutdown method#9
nnnkkk7 merged 2 commits intomainfrom
add/shutdown-method

Conversation

@nnnkkk7
Copy link
Contributor

@nnnkkk7 nnnkkk7 commented May 13, 2025

@nnnkkk7 nnnkkk7 requested a review from cre8ivejp as a code owner May 13, 2025 08:36
@nnnkkk7 nnnkkk7 requested a review from duyhungtnn June 12, 2025 06:52
Copy link
Collaborator

@duyhungtnn duyhungtnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nnnkkk7 nice PR. I forgot this one,
Android provider has this shutdown method too.

Can we add a unit test for the Shutdown() method?

@nnnkkk7 nnnkkk7 marked this pull request as draft June 12, 2025 09:50
@nnnkkk7 nnnkkk7 force-pushed the add/shutdown-method branch from 097b1a9 to 26ae283 Compare June 12, 2025 12:34
@nnnkkk7
Copy link
Contributor Author

nnnkkk7 commented Jun 12, 2025

@duyhungtnn
I tried it. But testing the Shutdown function with mocks wouldn't provide meaningful validation, so we'll add this test as part of our e2e test suite.

@nnnkkk7 nnnkkk7 marked this pull request as ready for review June 12, 2025 12:37
@duyhungtnn
Copy link
Collaborator

duyhungtnn commented Jun 12, 2025

@nnnkkk7 I’m not sure if we can easily verify the Shutdown method in E2E tests, but if it’s possible, please do.

Based on my previous implementation in the client provider:
To test the Shutdown() method without an E2E environment, I need an integration test. First, I create a provider and attach it to the OpenFeature SDK. Then, I can call a method to clear all providers from the OpenFeature SDK instance. At that point, I can verify whether our Shutdown method was called and whether it triggered Close() in our Bucketeer Client interface.

Do you think this is possible with Go?

@nnnkkk7 nnnkkk7 marked this pull request as draft June 13, 2025 02:59
@nnnkkk7
Copy link
Contributor Author

nnnkkk7 commented Jun 13, 2025

@duyhungtnn
Thank you!
You're right about integration tests. Since we currently only have unit tests with mocks, could we test the Shutdown method call using mocks for this scope? We can add integration tests in a separate PR.🙏

Copy link
Collaborator

@duyhungtnn duyhungtnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nnnkkk7 yes, I think its good.

@nnnkkk7 nnnkkk7 closed this Jun 18, 2025
@nnnkkk7 nnnkkk7 deleted the add/shutdown-method branch June 18, 2025 07:17
@nnnkkk7 nnnkkk7 restored the add/shutdown-method branch June 18, 2025 07:17
@nnnkkk7 nnnkkk7 reopened this Jun 18, 2025
@nnnkkk7 nnnkkk7 force-pushed the add/shutdown-method branch from 26ae283 to 482e486 Compare June 18, 2025 07:29
@nnnkkk7 nnnkkk7 marked this pull request as ready for review June 18, 2025 07:32
@nnnkkk7 nnnkkk7 merged commit 1ecccea into main Jul 2, 2025
5 checks passed
@nnnkkk7 nnnkkk7 deleted the add/shutdown-method branch July 2, 2025 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants